fix : Add request timeout to shell completion function to prevent indefinite waiting - #7794
fix : Add request timeout to shell completion function to prevent indefinite waiting#7794karthik120710 wants to merge 3 commits into
Conversation
Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
added
the
size/M
karmada-bot
added
the
do-not-merge/contains-merge-commits
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR updates karmadactl shell completion resource discovery to avoid hanging indefinitely when the API server is slow or unresponsive by introducing a 5-second timeout guard around the API-resources completion path.
Changes:
- Added a
completionRequestTimeoutconstant (5s). - Wrapped
compGetResourceListexecution in a goroutine and used aselectwith timeout to stop waiting past the deadline.
Comments suppressed due to low confidence (2)
pkg/karmadactl/util/completion/completion.go:312
- In the timeout/error paths, the code returns nil even though the comments say it returns an empty completion list. Returning an explicit empty slice better matches the documentation and avoids any nil-vs-empty edge cases in downstream completion handling.
var comps []string
resources := strings.SplitSeq(buf.String(), "\n")
for res := range resources {
if res != "" && strings.HasPrefix(res, suffix) {
comps = append(comps, fmt.Sprintf("%s%s", prefix, res))
}
}
pkg/karmadactl/util/completion/completion.go:268
- The timeout context is only used to stop waiting for the goroutine; it is never applied to the underlying discovery request (o.Complete/o.RunAPIResources). That means the API call itself can still block indefinitely and this change relies on returning early rather than enforcing a true request timeout (equivalent to --request-timeout=5s). Consider setting the REST config/discovery client timeout used by apiresources (e.g., wrap restClientGetter.ToDiscoveryClient to build a discovery client from a rest.Config with Timeout=completionRequestTimeout) so the request is actually bounded.
// compGetResourceList returns the list of api resources which begin with `toComplete`.
func compGetResourceList(restClientGetter genericclioptions.RESTClientGetter, cmd *cobra.Command, toComplete string) []string {
buf := new(bytes.Buffer)
streams := genericiooptions.IOStreams{In: os.Stdin, Out: buf, ErrOut: io.Discard}
// TODO: Using karmadactlapiresources.CommandAPIResourcesOptions to adapt to the operation scope.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Ignore errors as the output may still be valid | ||
| if err := o.RunAPIResources(); err != nil { | ||
| errorChan <- err | ||
| return | ||
| } |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7794 +/- ##
==========================================
- Coverage 42.09% 42.08% -0.02%
==========================================
Files 879 879
Lines 54853 54864 +11
==========================================
- Hits 23090 23087 -3
- Misses 30020 30033 +13
- Partials 1743 1744 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
|
hi @yanfeng1992 can u review these pr. |
Summary
This PR adds a 5-second timeout to the shell completion function compGetResourceList in completion.go to prevent indefinite waiting when the Karmada API server is slow or unresponsive.
Problem
The shell completion function previously had no timeout protection, which could cause the shell to hang indefinitely during command completion if the API server was slow or unresponsive. This provided a poor user experience.
Solution
Added a completionRequestTimeout constant set to 5 seconds
Modified compGetResourceList to use context.WithTimeout for timeout protection
Completion operations now run in a goroutine with channels for result/error communication
On timeout or error, the function returns nil to fail gracefully without hanging the shell
Removed the TODO comment that requested this feature
Changes
File: completion.go
Lines: Added constant at line 45, modified function at lines 249-313
Impact: Low risk - defensive measure that shouldn't affect normal operations
Testing
Code compiles successfully
Normal completion operations continue to work as expected
Timeout protection prevents shell hangs on slow/unresponsive API servers
#7793